Skip to content
This repository has been archived by the owner on Jan 29, 2025. It is now read-only.

[msl-out] wrap arrays in structs so that they can be returned by functions #764

Merged
merged 9 commits into from
Apr 28, 2021

Conversation

expenses
Copy link
Contributor

@expenses expenses commented Apr 23, 2021

This fixes #741. Unfortunately it currently breaks the quad-vert.msl snapshot with this error:

Validating quad-vert.msl
<stdin>:40:30: error: type 'type6' is not valid for attribute 'clip_distance'
    type6 gl_ClipDistance1 [[clip_distance]];
                             ^~~~~~~~~~~~~
<stdin>:42:8: error: invalid return type 'main2Output' for vertex function
vertex main2Output main2(
       ^
<stdin>:54:86: error: type 'type6' does not provide a subscript operator
    const auto _tmp = type10 {v_uv, _.gl_Position, _.gl_PointSize, {_.gl_ClipDistance[0]}};
                                                                    ~~~~~~~~~~~~~~~~~^~
3 errors generated.

This right solution to fix this test would be for gl_ClipDistance to be treated as a float instead of a float[1] in msl-out.

@expenses expenses changed the title [msl-out] wrap arrays in structs so that they can be returned by functions [msl-out][wip] wrap arrays in structs so that they can be returned by functions Apr 23, 2021
@expenses
Copy link
Contributor Author

This right solution to fix this test would be for gl_ClipDistance to be treated as a float instead of a float[1] in msl-out.

I guess we need some kind of type override system for this?

@kvark
Copy link
Member

kvark commented Apr 24, 2021

Thank you for taking a stab at this!

This right solution to fix this test would be for gl_ClipDistance to be treated as a float instead of a float[1] in msl-out.

Are you sure this is the right solution? I think there may be multiple clip distances, technically, so if we implement this workaround, we'll be back at square one with regards to general support for this.
Instead, the subject of this PR sounds more reasonable. I do wonder if we should simply wrap all of the arrays...

@expenses
Copy link
Contributor Author

Are you sure this is the right solution? I think there may be multiple clip distances, technically, so if we implement this workaround, we'll be back at square one with regards to general support for this.
Instead, the subject of this PR sounds more reasonable. I do wonder if we should simply wrap all of the arrays...

Yeah I just realised that table 5.3 of https://developer.apple.com/metal/Metal-Shading-Language-Specification.pdf states that it can be an array of floats. I'll have a go at using the array type in the output struct so instead of type6 gl_ClipDistance1 [[clip_distance]]; we have float[1] gl_ClipDistance1 [[clip_distance]];

@expenses
Copy link
Contributor Author

Instead, the subject of this PR sounds more reasonable. I do wonder if we should simply wrap all of the arrays...

To clarify, the reason we can't do is is because a wrapped array doesn't count for [[clip_distance]]:

<stdin>:40:30: error: type 'type6' is not valid for attribute 'clip_distance'
    type6 gl_ClipDistance1 [[clip_distance]];
                             ^~~~~~~~~~~~~

@expenses expenses changed the title [msl-out][wip] wrap arrays in structs so that they can be returned by functions [msl-out] wrap arrays in structs so that they can be returned by functions Apr 24, 2021
@expenses
Copy link
Contributor Author

I think what I've got here works pretty well now. Just FYI, what spirv-cross does in these situations is that it creates a template struct like:

template<typename T, size_t Num>
struct spvUnsafeArray
{
    T elements[Num ? Num : 1];
    
    thread T& operator [] (size_t pos) thread
    {
        return elements[pos];
    }
    constexpr const thread T& operator [] (size_t pos) const thread
    {
        return elements[pos];
    }
    
    device T& operator [] (size_t pos) device
    {
        return elements[pos];
    }
    constexpr const device T& operator [] (size_t pos) const device
    {
        return elements[pos];
    }
    
    constexpr const constant T& operator [] (size_t pos) const constant
    {
        return elements[pos];
    }
    
    threadgroup T& operator [] (size_t pos) threadgroup
    {
        return elements[pos];
    }
    constexpr const threadgroup T& operator [] (size_t pos) const threadgroup
    {
        return elements[pos];
    }
};

and returns that in functions instead:

static inline __attribute__((always_inline))
spvUnsafeArray<float, 1> xyz()
{
    spvUnsafeArray<float, 1> y;
    return y;
}

This means that they don't have to do .inner at all.

Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing stuff, I'm excited to see this happening!
I'd prefer to make this work with inner instead of defining this complex template in the code like SPIRV-Cross does.

src/back/msl/writer.rs Show resolved Hide resolved
src/back/msl/writer.rs Outdated Show resolved Hide resolved
src/back/msl/writer.rs Outdated Show resolved Hide resolved
src/back/msl/writer.rs Show resolved Hide resolved
src/back/msl/writer.rs Show resolved Hide resolved
@expenses expenses requested a review from kvark April 26, 2021 07:40
@kvark
Copy link
Member

kvark commented Apr 26, 2021

Are we all set?

const auto _tmp = type10 {v_uv, _.gl_Position, _.gl_PointSize, {_.gl_ClipDistance[0]}};
return main2Output { _tmp.member, _tmp.gl_Position1, _tmp.gl_PointSize1, {_tmp.gl_ClipDistance1[0]} };
const auto _tmp = type10 {v_uv, _.gl_Position, _.gl_PointSize, _.gl_ClipDistance};
return main2Output { _tmp.member, _tmp.gl_Position1, _tmp.gl_PointSize1, {_tmp.gl_ClipDistance1.inner[0]} };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait, so here we can't just do _tmp.gl_ClipDistance1 for the last argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, unfortunately not. Not gl_ClipDistance1.inner either.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, I'm concerned now, a little bit. So when we are initializing the I/O struct, we can't just pass _tmp.gl_ClipDistance1 because it's an array. But how would this work in other places? I.e. if you have a user function returning a user struct, then passing { xxx.inner[0], xxx.inner[1] } would just fail horribly, since the only field of the target struct is inner.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Afaik, only having a single pair of those { brackets when constructing a struct is okay if it only has one field. I think more testing would be nice, but I haven't found a (trivial) example that this breaks. Something like the following, for example, is fine:

#version 450

vec3[8] hello_world() {
    vec3[8] xyz;
    return xyz;
}

struct X {
  vec3[8] yyy;
};

vec3[8] hmm2() {
  X x;
  x.yyy = hello_world();
  x.yyy[1] = vec3(3.0);
  return x.yyy;
}

layout(location = 0) out vec4 colour;

void main() {
    vec3[8] xyz = hmm2();
    colour = vec4(xyz[7], 1.0);
}

->

#include <metal_stdlib>
#include <simd/simd.h>

struct type3 {
    metal::float3 inner[8u];
};
struct X {
    type3 yyy;
};
constant metal::float3 const_type1_ = {3.0, 3.0, 3.0};

type3 hello_world(
) {
    type3 xyz;
    return xyz;
}

type3 hmm2_(
) {
    X x;
    type3 _e13 = hello_world();
    for(int _i=0; _i<8; ++_i) x.yyy.inner[_i] = _e13.inner[_i];
    x.yyy.inner[1] = const_type1_;
    return x.yyy;
}

void main1(
    thread metal::float4& colour
) {
    type3 xyz1;
    type3 _e13 = hmm2_();
    for(int _i=0; _i<8; ++_i) xyz1.inner[_i] = _e13.inner[_i];
    metal::float3 _e15 = xyz1.inner[7];
    colour = metal::float4(_e15.x, _e15.y, _e15.z, 1.0);
    return;
}

struct main2Output {
    metal::float4 member [[color(0)]];
};
fragment main2Output main2(
) {
    metal::float4 colour = {};
    main1(colour);
    return main2Output { colour };
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, specifically for arrays.

struct Foo {
  xxx: array<f32; 3>;
};
fn bar() -> Foo {
  return Foo(array<f32;3>(1.0, 2.0, 3.0));
}

My understanding is that this PR right now is not going to handle this. Ideally, we'd want it to generate something like

const auto _tmp = Foo { .. };
return { { _tmp.xxx[0], _tmp.xxx[1], _tmp.xxx[2] } };

This will fail because the produced Foo would be declared with the inner in it, but the return code doesn't use it.

Copy link
Member

@kvark kvark Apr 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I don't understand how type1 {1.0, 2.0, 3.0} part works :(
type1 is a structure with 1 element. So shouldn't it yell at us for providing 3 elements instead of 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'm not 100% sure either, but what seems to happen is that (as I wrote in #764 (comment)):

Afaik, only having a single pair of those { brackets when constructing a struct is okay if it only has one field

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing is allowed in C:

struct Arr {
    float inner[4];
};

int main() {
    struct Arr arr = { 1.0, 1.5, 2.0, 2.5 };
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh oh, in both MSL and C it lets you skip fields in the constructor though so

Foo bar1(
) {
    return Foo {type1 {1.0}, 2};
}

also works?? With no warnings or errors 😟 ??

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's going to happen if the array only has one element? How is the compiler going to figure out if the initializer is for this field or for the inner itself? And initializers don't even have to be provided for all the fields. Ugh, C is weird :(

@expenses
Copy link
Contributor Author

Are we all set?

I think so! I hope this doesn't break any vertex shaders.

@kvark
Copy link
Member

kvark commented Apr 28, 2021

I'm royally confused about how C arrays work, and that is going to make the maintenance of this code to be difficult.
But you've really done a great job implementing it, and it addresses a very important issue we have, and there isn't any cleaner solution that we know. So let's proceed!

@kvark kvark merged commit d21dded into gfx-rs:master Apr 28, 2021
jimblandy pushed a commit to jimblandy/naga that referenced this pull request May 9, 2023
Assigning arrays by value works fine since all arrays are now wrapped by a struct.
jimblandy pushed a commit to jimblandy/naga that referenced this pull request May 9, 2023
Assigning arrays by value works fine since all arrays are now wrapped by a struct.
jimblandy added a commit that referenced this pull request May 9, 2023
Assigning arrays by value works fine since all arrays are now wrapped by a struct.

Co-authored-by: teoxoy <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Metal] functions cannot return arrays
2 participants